Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Dimensions2d class #54

Merged
merged 29 commits into from
Jan 5, 2021
Merged

Add Dimensions2d class #54

merged 29 commits into from
Jan 5, 2021

Conversation

angusm
Copy link
Member

@angusm angusm commented Dec 15, 2020

No description provided.

@jeremydw jeremydw requested a review from uxder December 15, 2020 00:14
src/mathf/dimensions-2d.ts Outdated Show resolved Hide resolved
src/mathf/dimensions-2d.ts Outdated Show resolved Hide resolved
src/dom/root-element.ts Outdated Show resolved Hide resolved
Copy link
Member

@uxder uxder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@angusm it looks good.

I see some duplication with the yano IS class. So check that out if you haven't yet.

I'm okay with adding the user-agent stuff since it in a subdirectory /user-agent and might be good for cases where you just want pieces.

Please check this since you need it but could you later go back and add some documentation for some of the clases? Just a comment at the top of each class and some quick examples helps us read through it.

@angusm
Copy link
Member Author

angusm commented Dec 17, 2020

Went through with aims to consolidate and comment.

src/arrayf/arrayf.ts Outdated Show resolved Hide resolved
src/dom/position/matrix.ts Outdated Show resolved Hide resolved
Copy link
Member

@uxder uxder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like x/x.ts was deleted.

@uxder
Copy link
Member

uxder commented Dec 17, 2020

@angusm Nice work. I think some of these classes are useful.

One overall comment, I have is that a lot of the caching classes might come at the cost of too much abstraction making it harder for others to navigate through code without heavily investing learning the library.

Part of me says, we should keep it simple and use straight native javascript read / writes combined in raf.read / raf.write - which isn't as optimized but simpler and easier to follow. However, I do see a real need to cache things like scroll, window so I see those as very useful.

I think this is good to go but since it does come at a cost of abstraction, just want to make sure we continue to make efforts to add docs / examples / tests to ease the cost of onboarding for others.

Thank you!

@uxder
Copy link
Member

uxder commented Dec 17, 2020

@angusm Nice work. I think some of these classes are useful.

One overall comment, I have is that a lot of the caching classes might come at the cost of too much abstraction making it harder for others to navigate through code without heavily investing learning the library.

Part of me says, we should keep it simple and use straight native javascript read / writes combined in raf.read / raf.write - which isn't as optimized but simpler and easier to follow. However, I do see a real need to cache things like scroll, window so I see those as very useful.

I think this is good to go but since it does come at a cost of abstraction, just want to make sure we continue to make efforts to add docs / examples / tests to ease the cost of onboarding for others.

Thank you!

@jeremydw @stevenle - have any thoughts on this?

@angusm
Copy link
Member Author

angusm commented Dec 17, 2020

Would adding helper functions to dom like there is currently for getStyle rather than using the singletons directly help make this easier to work with?

Sort of bury the caching in a black box users don't need to worry about, beyond "use these functions instead of native calls for best results"?

src/arrayf/arrayf.ts Outdated Show resolved Hide resolved
src/arrayf/arrayf.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/iterable-iterator/map.ts Outdated Show resolved Hide resolved
src/dom/webgl-image-sequence.ts Outdated Show resolved Hide resolved
src/mathf/geometry/multi-dimensional-vector.ts Outdated Show resolved Hide resolved
@jeremydw
Copy link
Member

Just to make sure I'm chiming in and adding my bits. This is general feedback I want us all to apply to get on the same page with our code style and approach. :)

  • Generally, create fewer files, with code grouped logically into the same file works best. Fewer files should hopefully reduce cognitive overhead when trying to determine where to go to update something, or learn how something works.
  • Avoid reimplementing things that are available from the standard library.
  • Avoid too much inheritance. I think inheritance can be really clever in certain cases but often for contributors it will require someone to have to trace through many files to read the code end to end. If a concept can be expressed with a standard "or" statement (picking on DynamicDefaultMap here) then I think the "or" statement is preferred. For DynamicDefaultMap it's pretty hard for me to follow.
  • I recently wrote a Contributing guide for Amagaki that I think can apply here to Degu (new name) too. I'll work to send a PR that adapts the Contributing guide for Degu, but I think it has some good learnings: https://github.com/blinkkcode/amagaki/blob/main/CONTRIBUTING.md#philosophy -- my favorite takeaway is "Write as little code as possible, but as much as necessary to complete a task correctly."

@angusm
Copy link
Member Author

angusm commented Dec 18, 2020

Let me re-work DynamicDefaultMap. It's meant to behave as defaultdict from python.
https://www.accelebrate.com/blog/using-defaultdict-python#:~:text=A%20defaultdict%20works%20exactly%20like,returned%20by%20the%20default%20factory.

The inheritance for these map classes was such a mess because at one point ES6 Map wouldn't allow itself to be extended directly. microsoft/TypeScript#10853 That seems to have been fixed in recent browser updates :) so this can be made much less messy.

Can you be specific about Avoid reimplementing things that are available from the standard library.?

I agree fully with Write as little code as possible, but as much as necessary to complete a task correctly., all of this code has been necessary at some point, it was left in to try and be pro-active against future needs, but I will go through and remove any pieces not currently in active use or needed immediately.

src/dom/cached-element-vector.ts Outdated Show resolved Hide resolved
src/mathf/geometry/multi-dimensional-vector.ts Outdated Show resolved Hide resolved
Remove sticky specific functionality.
Copy link
Member

@uxder uxder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few more comments.

src/dom/dimensions-2d-dom.ts Outdated Show resolved Hide resolved
src/map/default.ts Outdated Show resolved Hide resolved
src/dom/get-visible-y-from-root.ts Outdated Show resolved Hide resolved
src/dom/get-visible-y-from-root.ts Outdated Show resolved Hide resolved
src/dom/dom.ts Outdated Show resolved Hide resolved
@stevenle
Copy link
Member

@uxder, i dunno what your file naming conventions are, but i'm thinking maybe map/default.ts should be renamed map/default-map.ts since the term "default" can have a confusing meaning here. wdyt?

src/dom/matrix-dom.ts Outdated Show resolved Hide resolved
@angusm
Copy link
Member Author

angusm commented Dec 19, 2020

@uxder, i dunno what your file naming conventions are, but i'm thinking maybe map/default.ts should be renamed map/default-map.ts since the term "default" can have a confusing meaning here. wdyt?

Yes. Done.

src/dom/dimensions-2d-dom.ts Outdated Show resolved Hide resolved
@uxder uxder merged commit af0b84d into blinkk:master Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants